(feat): igraph leiden implementation now included as an option in sc.tl.leiden
#2815
Conversation
ilan-gold
commented
Jan 16, 2024
•
ilan-gold
commented
- Closes Leiden now included in python-igraph #1053
- Tests included or not required because:
- Release notes not necessary because:
|
TODOs:
|
|
Failing |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2815 +/- ##
==========================================
+ Coverage 74.57% 74.63% +0.05%
==========================================
Files 115 115
Lines 12716 12756 +40
==========================================
+ Hits 9483 9520 +37
- Misses 3233 3236 +3
|
scanpy/tools/_leiden.py
Outdated
| clustering_args["weights"] = ( | ||
| "weight" if use_igraph else np.array(g.es["weight"]).astype(np.float64) | ||
| ) |
There was a problem hiding this comment.
I think both of these should be added to uns as outputs, weights in the params part of the dict and use_igraph at the top levels
adata['leiden']
# { 'params': { ... 'use_weights': True }, 'use_igraph': False }
ilan-gold
changed the title
igraph leiden implementation is now the defaultigraph leiden implementation now included as an option in sc.tl.leiden
There was a problem hiding this comment.
There are a lot of branches here like if flavor == "leidenalg", I think it could be cleaner to just have one higher level branch for the two cases.
I think we can change the default for directed to be None then dynamically set the value based on which flavor is passed.
We could do the same with n_iterations, though I'm a little on the fence about this.
A number of plots are being changed that don't seem to be very different. Mostly things pre-clustering, like pca and qc metrics. I'm going to check if these are actually different enough to trigger test failures.
|
@ivirshup You are right about some of the pre-processing plots. I should have created a separate branch for those. I simply think hte outputs have changed and we never caught it but would be curious to see what you get. It could be my M1 |
|
@ivirshup I simplified the conditionals a bit and there are only two sets now. One to check for various |
|
About the preprocessing plots:
|
|
@ivirshup I will revert those, and hopefully tests pass |
There was a problem hiding this comment.
Looks good! But I don't think that you addressed these:
I think we can change the default for directed to be None then dynamically set the value based on which flavor is passed.
We could do the same with n_iterations, though I'm a little on the fence about this.
I think I would definitely change directed, and add a note in the docs that setting n_iterations=2 will be faster and is the default for the underlying packages.
|
Ah, you're right. Will change. |
|
Hi there @ilan-gold @ivirshup, I just wanted to get some clarification on something. Suppose I have the following bit of code: Should it return the same results with |
|
@alam-shahul can you create an issue with a reproducer? |
|
@ilan-gold Sorry for the delay, here you go: #3422 |
